Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FrSky new sensors support #5766

Merged
merged 4 commits into from Mar 17, 2018
Merged

FrSky new sensors support #5766

merged 4 commits into from Mar 17, 2018

Conversation

bsongis
Copy link
Member

@bsongis bsongis commented Mar 14, 2018

Don't merge it now, some other sensors are still missing

@3djc
Copy link
Contributor

3djc commented Mar 15, 2018

I have added some more sensors. There is an issue with the gas flow sensor : we do not have flow unit. Adding a flow unit would require conversion (because if added after text, if would fail validity test < UNIT_FIRST_VIRTUAL in a couple of places)

{ ESC_RPM_CONS_FIRST_ID, ESC_RPM_CONS_LAST_ID, 0, ZSTR_ESC_RPM, UNIT_RPMS, 0 },
{ ESC_RPM_CONS_FIRST_ID, ESC_RPM_CONS_LAST_ID, 1, ZSTR_ESC_CONSUMPTION, UNIT_MAH, 0 },
{ ESC_TEMPERATURE_FIRST_ID, ESC_TEMPERATURE_LAST_ID, 0, ZSTR_ESC_TEMP, UNIT_CELSIUS, 0 },
{ GASSUIT_TEMP_FIRST_ID, GASSUIT_TEMP_LAST_ID, 0, ZSTR_TEMP1, UNIT_CELSIUS, 0 },
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure we can reuse ZSTR in multiple sensors here (unless we decide that GasSuite is not used with a temperature sensor)

else if (id >= ESC_TEMPERATURE_FIRST_ID && id <= ESC_TEMPERATURE_LAST_ID) {
sportProcessTelemetryPacket(id, 0, instance, data & 0x00ff);
}
else if (id >= GASSUIT_TEMP_FIRST_ID && id <= GASSUIT_TEMP_LAST_ID) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those 2 cases can be combined (as I did for ESC), but perhaps the compiler does the optimization on his own
in fact, the comment is right, there are 2 sensors but you didn't combine them

@bsongis
Copy link
Member Author

bsongis commented Mar 17, 2018

Thanks again for your help @3djc this seems perfect to me, we just need some sensors to test the feature. I will try to have a sensors set for you

@bsongis bsongis merged commit 2b604a4 into 2.2 Mar 17, 2018
@bsongis bsongis deleted the bsongis/frsky_new_sensors branch March 17, 2018 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants